Website: Hero video optimization (subtitles, flickering) #944
Conversation
mdumond
requested review from
andrashee,
mkue,
ssandino and
socialincome-dev
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new key, Changes
Possibly related issues
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit bf69270): https://si-admin-staging--pr944-934-video-subtitle-t-zrosyjgd.web.app (expires Tue, 10 Dec 2024 18:58:54 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
website/src/app/[lang]/[region]/(website)/(home)/(sections)/hero-video.tsx (2)
16-18: Add type safety for subtitle translationsConsider adding type definitions for the translations prop to ensure type safety and better IDE support.
+interface VideoTranslations { + subtitles: string; +} <MuxVideoComponent translations={{ - subtitles: translator.t<string>('video-subtitle'), + subtitles: translator.t<VideoTranslations>('video-subtitle').subtitles, }}
15-21: Regarding language name handlingIn response to your question about language names: Instead of passing just the
langcode, you might want to use the built-inIntl.DisplayNamesAPI to get the language name. This would provide consistent language name formatting across browsers.Example implementation:
const languageName = new Intl.DisplayNames([lang], { type: 'language' }).of(lang); <MuxVideoComponent translations={{ subtitles: translator.t<string>('video-subtitle'), }} lang={lang} region={region} languageLabel={languageName} // Add this prop to MuxVideoComponent />website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx (1)
Line range hint
88-94: Consider improving subtitle styling maintainability.The current subtitle styling could be enhanced for better maintainability and responsiveness:
Consider these improvements:
- <style>{` - video::cue { - background-color: rgba(0, 0, 0, 0.8); - color: white; - font-family: Arial, sans-serif; - font-size: 24px; - opacity: ${showCaptions ? 1 : 0}; - `}</style> + <style>{` + video::cue { + background-color: var(--subtitle-bg, rgba(0, 0, 0, 0.8)); + color: var(--subtitle-color, white); + font-family: var(--subtitle-font, Arial, sans-serif); + font-size: clamp(16px, 2vw, 24px); + opacity: ${showCaptions ? 1 : 0}; + } + `}</style>This change:
- Makes font size responsive using
clamp- Uses CSS variables for easier theming
- Improves maintainability by centralizing style values
shared/locales/en/website-home.json (1)
23-23: Consider restructuring subtitle configuration.The subtitle path is currently at the root level of the JSON. Consider moving it under a dedicated "video" or "media" section for better organization and maintainability.
{ + "media": { + "video-subtitle": "/assets/video/subtitle-video-testimonials.en_US.vtt" + }, "metadata": { ... }, - "video-subtitle": "/assets/video/subtitle-video-testimonials.en_US.vtt", "section-1": { ... }shared/locales/de/website-home.json (1)
23-23: Consider reorganizing the JSON structureThe
video-subtitlekey is placed between content sections. Consider moving it to a more logical location, such as under themetadatasection or creating a dedicatedmediasection for better organization and maintainability.{ "metadata": { "title": "Social Income", "description": "Change from human to human. From mobile to mobile.", "og-image": "/assets/metadata/og/default.jpg", - "twitter-image": "/assets/metadata/twitter/default.jpg" + "twitter-image": "/assets/metadata/twitter/default.jpg", + "video-subtitle": "/assets/video/subtitle-video-testimonials.de_DE.vtt" }, "section-1": {shared/locales/it/website-home.json (1)
23-23: Consider adding a comment to document the subtitle path structure.To maintain consistency across locales, consider adding a comment in the JSON file to document the expected format of the subtitle path:
+ // Format: /assets/video/subtitle-video-testimonials.[langcode]_[REGION].vtt "video-subtitle": "/assets/video/subtitle-video-testimonials.it_IT.vtt",shared/locales/fr/website-home.json (1)
23-23: Consider adding language display names for accessibility.Since the PR mentions uncertainty about retrieving language names from codes, consider adding a
subtitle-languagekey to display user-friendly language names (e.g., "Français") in the video player's subtitle menu for better accessibility.{ + "video-subtitle-language": "Français", "video-subtitle": "/assets/video/subtitle-video-testimonials.fr_FR.vtt",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
shared/locales/de/website-home.json(1 hunks)shared/locales/en/website-home.json(1 hunks)shared/locales/fr/website-home.json(1 hunks)shared/locales/it/website-home.json(1 hunks)website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx(3 hunks)website/src/app/[lang]/[region]/(website)/(home)/(sections)/hero-video.tsx(1 hunks)
🔇 Additional comments (8)
website/src/app/[lang]/[region]/(website)/(home)/(sections)/hero-video.tsx (1)
15-21: Verify the translation key exists across all language files
The code retrieves subtitles using the 'video-subtitle' key. Let's ensure this key is properly defined in all language files.
✅ Verification successful
Translation key 'video-subtitle' is properly defined across all language files
The verification confirms that the 'video-subtitle' key exists in all language files (de, en, fr, it), ensuring proper internationalization support for the video subtitles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'video-subtitle' key exists in all language files
# Find all website-home.json files and check for the key
fd --glob 'website-home.json' shared/locales | while read -r file; do
echo "Checking $file..."
jq 'has("video-subtitle")' "$file"
done
Length of output: 819
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx (3)
13-17: LGTM! Well-structured type definition.
The HeroVideoSubtitles type is cleanly defined and properly extends DefaultParams using intersection types.
19-19: LGTM! Component signature properly updated.
The component signature has been correctly updated to accept language and translation props with proper typing.
87-87:
Fix hardcoded language label in track element.
The label attribute is hardcoded as "English" while the srcLang is dynamic. This could be confusing for users when viewing subtitles in other languages.
Let's check if there's an existing language name utility in the codebase:
Consider updating the track element to use the correct language name:
- <track kind="captions" src={translations.subtitles} srcLang={lang} label="English" default />
+ <track kind="captions" src={translations.subtitles} srcLang={lang} label={getLanguageName(lang)} default />If no existing utility is found, you might want to add one using the ISO 639-1 standard language codes. I can help implement this if needed.
✅ Verification successful
Let me search for language name translations in the locales directory:
Let me try searching for language translations in a different way:
Let me check the content of the common.json files to see the language translations:
Based on the findings, I can now generate the final response:
Use the existing i18n system for the track label
The codebase already has a translation system in place with language names. You can use the existing translator instance to get the language name:
- <track kind="captions" src={translations.subtitles} srcLang={lang} label="English" default />
+ <track kind="captions" src={translations.subtitles} srcLang={lang} label={translator.t(`languages.${lang}`)} default />🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for language name/code conversion utilities
rg -g '!*.test.*' -g '!*.spec.*' "langcode|language.*name|language.*code" -A 5
Length of output: 18888
Script:
#!/bin/bash
# Search for language translations in locales
fd "languages.json" --exec cat {} \;
Length of output: 39
Script:
#!/bin/bash
# Search for language translations in locales directory
rg -g '*.json' '"languages"' shared/locales
Length of output: 229
Script:
#!/bin/bash
# Check content of common.json files for language translations
fd "common.json" shared/locales --exec cat {} \; | grep -A 10 '"languages":'
Length of output: 760
shared/locales/en/website-home.json (1)
23-23: Verify subtitle file existence and structure.
The path references a VTT subtitle file. Let's ensure it exists and follows the same pattern across all locales.
✅ Verification successful
Subtitle files and naming patterns are consistent across locales
The verification shows:
- All referenced VTT subtitle files exist in the correct location (
website/public/assets/video/) - Consistent naming pattern
subtitle-video-testimonials.[lang]_[REGION].vttis followed across all locales - Each locale file correctly references its corresponding VTT file with matching language code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify subtitle files existence and naming consistency
# Check if the subtitle files exist for all locales
echo "Checking subtitle files existence..."
fd -e vtt -p "assets/video/subtitle-video-testimonials.*"
# Verify consistent naming pattern across locale files
echo "Checking naming pattern in locale files..."
rg -U --multiline '"video-subtitle":\s*"([^"]+)"' shared/locales/
Length of output: 1075
shared/locales/de/website-home.json (1)
23-23: Verify the existence of the subtitle file
The path references a VTT subtitle file. Let's verify its existence and proper formatting.
✅ Verification successful
Subtitle file exists and is properly formatted
The VTT file exists at the correct location (website/public/assets/video/), has proper WEBVTT formatting, and the path in the JSON file correctly references it relative to the public assets directory. The implementation is consistent with other language variants (en_US, fr_FR, it_IT).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the subtitle file exists and validate its format
# Check if the file exists
fd -t f "subtitle-video-testimonials.de_DE.vtt" public/assets/video
# If found, verify basic VTT format (should start with "WEBVTT")
fd -t f "subtitle-video-testimonials.de_DE.vtt" public/assets/video -x head -n 1 {} \; | grep "WEBVTT"
Length of output: 385
Script:
#!/bin/bash
# Let's first check if the assets directory exists and its location
fd -t d "assets"
# Then search for any VTT files in the repository
fd -t f -e vtt
# Also search for references to VTT files in the codebase
rg "\.vtt" -g '!*.vtt'
Length of output: 1373
Script:
#!/bin/bash
# Let's verify the content of the German VTT file
head -n 5 website/public/assets/video/subtitle-video-testimonials.de_DE.vtt
# Check if the file path in the JSON matches the actual file location
rg -A 1 '"video-subtitle"' shared/locales/de/website-home.json
Length of output: 308
shared/locales/it/website-home.json (1)
23-23: Verify the subtitle file existence and format.
The path follows the correct naming convention. Let's verify the implementation.
✅ Verification successful
✓ Subtitle file exists and follows the correct structure
The verification confirms:
- The subtitle file
subtitle-video-testimonials.it_IT.vttexists at the correct location:website/public/assets/video/ - The implementation is consistent with other locales (de_DE, en_US, es_ES, fr_FR also present)
- The path in the locale file correctly points to the public assets directory
- The MuxVideoComponent correctly consumes the subtitle path via translations prop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and format of the subtitle file
# Check if the subtitle file exists
fd "subtitle-video-testimonials.it_IT.vtt" assets/video
# Validate WebVTT format if file exists
fd "subtitle-video-testimonials.it_IT.vtt" assets/video -x cat {} | head -n 1 | grep -q "^WEBVTT" && echo "Valid WebVTT file" || echo "Invalid or missing WebVTT file"
# Check for consistency across all locales
fd "subtitle-video-testimonials.*\.vtt" assets/video
Length of output: 581
Script:
#!/bin/bash
# Let's first check if the assets directory exists and its location
fd -t d "assets"
# Check for any .vtt files in the repository
fd -e vtt
# Let's also check the MuxVideoComponent to understand the expected structure
rg -A 5 "MuxVideoComponent"
Length of output: 3123
shared/locales/fr/website-home.json (1)
23-23: Verify the subtitle file existence and format.
The path looks correct, following proper naming conventions with language-region code (fr_FR). However, let's verify:
- The existence of the subtitle file
- The consistency of paths across other locale files
- The validity of the VTT format
✅ Verification successful
✓ Subtitle file path and structure verified
The French subtitle file exists at the correct location website/public/assets/video/subtitle-video-testimonials.fr_FR.vtt, along with other language variants. The path in the locale file /assets/video/subtitle-video-testimonials.fr_FR.vtt is correct as it's relative to the public directory, following the same consistent pattern across all locales (en_US, de_DE, fr_FR, it_IT, es_ES).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify subtitle file existence, consistency, and format
# Check if the French subtitle file exists
fd "subtitle-video-testimonials.fr_FR.vtt" public/assets/video
# Check naming consistency across all locale files
rg -U '"video-subtitle":\s*"([^"]+)"' shared/locales
# Validate VTT format if file exists
fd "subtitle-video-testimonials.fr_FR.vtt" public/assets/video -x cat {} | grep -E "^WEBVTT|^[0-9]{2}:[0-9]{2}:[0-9]{2}"
Length of output: 894
Script:
#!/bin/bash
# Let's try searching for the subtitle files in the entire repository
# and check the assets directory structure
# Find any .vtt files
fd ".+\.vtt$"
# Check for potential asset directories
fd -t d "assets|public"
# Look for any build/asset configuration files
fd -e json "package.json|next.config.js|vite.config.js|webpack.config.js"
# Search for references to subtitle files or video assets in code
rg -g "!*.json" "subtitle-video-testimonials|assets/video"
Length of output: 1247
|
@mathildedd @mkue we got the answer from the Mux support team who looked at this PR: Great to hear! So I see you're using the Mux Video element, the handling of captions would be under the vid element as you have. However, when I look and inspect that video element on your landing page, I'm only seeing the English subtitle in there: Also, the Mux Video should attach all subtitles to the video element if you add the subtitles to your Mux Asset. You could then programmatically set the captions language with something like this: |
ssandino
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx (1)
106-113: Move subtitle styles to CSS moduleInline styles make maintenance harder and mix presentation with logic. Consider moving these styles to a CSS module.
Create a new file
mux-video.module.css:.subtitle { background-color: rgba(0, 0, 0, 0.8); color: white; font-family: Arial, sans-serif; font-size: 24px; }Then update the component:
-<style>{` - video::cue { - background-color: rgba(0, 0, 0, 0.8); - color: white; - font-family: Arial, sans-serif; - font-size: 24px; - opacity: ${showCaptions ? 1 : 0}; - } -`}</style> +<style>{` + video::cue { + opacity: ${showCaptions ? 1 : 0}; + } +`}</style>
| const video = videoElementRef.current; | ||
| if (playing && video) { | ||
| // Hide poster when video is ready | ||
| video.addEventListener('canplay', () => { | ||
| if (posterRef.current) { | ||
| posterRef.current.style.opacity = '0'; | ||
| posterRef.current.style.transition = 'opacity 0.5s ease'; | ||
| } | ||
| }); | ||
| video.play(); |
There was a problem hiding this comment.
Add cleanup for canplay event listener
The canplay event listener is not cleaned up when the component unmounts, which could lead to memory leaks.
useEffect(() => {
const video = videoElementRef.current;
if (playing && video) {
- video.addEventListener('canplay', () => {
+ const handleCanPlay = () => {
if (posterRef.current) {
posterRef.current.style.opacity = '0';
posterRef.current.style.transition = 'opacity 0.5s ease';
}
- });
+ };
+ video.addEventListener('canplay', handleCanPlay);
video.play();
+ return () => video.removeEventListener('canplay', handleCanPlay);
} else {
videoElementRef.current?.pause();
}
}, [playing]);Committable suggestion skipped: line range outside the PR's diff.
| label="English" | ||
| default | ||
| /> | ||
| <track kind="captions" src={translations.subtitles} srcLang={lang} label="English" default /> |
There was a problem hiding this comment.
Fix hardcoded language label in track element
The track element uses a hardcoded "English" label while the component accepts dynamic language props. This could lead to confusion for users of other languages.
-<track kind="captions" src={translations.subtitles} srcLang={lang} label="English" default />
+<track kind="captions" src={translations.subtitles} srcLang={lang} label={lang.toUpperCase()} default />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <track kind="captions" src={translations.subtitles} srcLang={lang} label="English" default /> | |
| <track kind="captions" src={translations.subtitles} srcLang={lang} label={lang.toUpperCase()} default /> |
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx
Outdated
Show resolved
Hide resolved
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx (1)
120-127: Consider extracting subtitle styles to configurationThe subtitle styles could be moved to a configuration file for better maintainability.
+// subtitleConfig.ts +export const subtitleStyles = { + backgroundColor: 'rgba(0, 0, 0, 0.8)', + color: 'white', + fontFamily: 'Arial, sans-serif', + fontSize: '24px', +}; - video::cue { - background-color: rgba(0, 0, 0, 0.8); - color: white; - font-family: Arial, sans-serif; - font-size: 24px; - opacity: ${showCaptions ? 1 : 0}; - } + video::cue { + background-color: ${subtitleStyles.backgroundColor}; + color: ${subtitleStyles.color}; + font-family: ${subtitleStyles.fontFamily}; + font-size: ${subtitleStyles.fontSize}; + opacity: ${showCaptions ? 1 : 0}; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx(4 hunks)
🔇 Additional comments (3)
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx (3)
13-19: LGTM: Clean type definition and component signature
The type definition and component signature are well-structured and properly typed.
20-33: LGTM: Proper error handling implementation
The error handling implementation follows the previous review suggestion with proper state management and cleanup.
57-68: LGTM: Proper event listener cleanup
The canplay event handler implementation follows the previous review suggestion with proper cleanup.
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx
Show resolved
Hide resolved
|
About subtitles not working on chrome, see muxinc/elements#941 and muxinc/elements#519 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx(3 hunks)
🔇 Additional comments (2)
website/src/app/[lang]/[region]/(website)/(home)/(components)/mux-video.tsx (2)
14-20: LGTM: Type definition and component signature
The type definition and component signature changes are well-structured for handling localized subtitles.
21-34: LGTM: Error handling implementation
The error handling implementation follows the previous review suggestion with proper cleanup.
| <track kind="captions" src={translations.subtitles} srcLang={lang} label={lang.toUpperCase()} default /> | ||
| <style>{` | ||
| video::cue { | ||
| background-color: rgba(0, 0, 0, 0.8); | ||
| color: white; | ||
| font-family: Arial, sans-serif; | ||
| font-size: 24px; | ||
| opacity: ${showCaptions ? 1 : 0}; | ||
| `}</style> | ||
| video::cue { | ||
| background-color: rgba(0, 0, 0, 0.8); | ||
| color: white; | ||
| font-family: Arial, sans-serif; | ||
| font-size: 24px; | ||
| opacity: ${showCaptions ? 1 : 0}; | ||
| } | ||
| `}</style> |
There was a problem hiding this comment.
💡 Codebase verification
Missing subtitle files need to be added
The translation files reference subtitle VTT files at /assets/video/, but these files are not present in the repository:
/assets/video/subtitle-video-testimonials.de_DE.vtt/assets/video/subtitle-video-testimonials.en_US.vtt/assets/video/subtitle-video-testimonials.fr_FR.vtt/assets/video/subtitle-video-testimonials.it_IT.vtt
🔗 Analysis chain
Verify subtitle file availability
The subtitle implementation looks correct, but we need to verify the subtitle files exist at the paths specified in translations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if subtitle files exist in public assets
fd -e vtt . public/assets/video
# Check subtitle paths in translation files
rg "video-subtitle.*\.vtt" shared/locales
Length of output: 624
Script:
#!/bin/bash
# Check if subtitle files exist in public directory
fd -e vtt subtitle-video-testimonials public
Length of output: 140
| const video = videoElementRef.current; | ||
| if (playing && video) { | ||
| // Hide poster when video is ready | ||
| const handleCanPlay = () => { | ||
| if (posterRef.current) { | ||
| posterRef.current.style.opacity = '0'; | ||
| posterRef.current.style.transition = 'opacity 0.5s ease'; | ||
| } | ||
| }; | ||
| video.addEventListener('canplay', handleCanPlay); | ||
| video.play(); | ||
| return () => video.removeEventListener('canplay', handleCanPlay); |
There was a problem hiding this comment.
Fix potential race condition in video playback
There's a potential race condition between video.play() and the canplay event listener. The video might start playing before the poster fade-out is complete.
Apply this fix:
if (playing && video) {
const handleCanPlay = () => {
if (posterRef.current) {
posterRef.current.style.opacity = '0';
posterRef.current.style.transition = 'opacity 0.5s ease';
}
+ video.play();
};
video.addEventListener('canplay', handleCanPlay);
- video.play();
return () => video.removeEventListener('canplay', handleCanPlay);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const video = videoElementRef.current; | |
| if (playing && video) { | |
| // Hide poster when video is ready | |
| const handleCanPlay = () => { | |
| if (posterRef.current) { | |
| posterRef.current.style.opacity = '0'; | |
| posterRef.current.style.transition = 'opacity 0.5s ease'; | |
| } | |
| }; | |
| video.addEventListener('canplay', handleCanPlay); | |
| video.play(); | |
| return () => video.removeEventListener('canplay', handleCanPlay); | |
| const video = videoElementRef.current; | |
| if (playing && video) { | |
| // Hide poster when video is ready | |
| const handleCanPlay = () => { | |
| if (posterRef.current) { | |
| posterRef.current.style.opacity = '0'; | |
| posterRef.current.style.transition = 'opacity 0.5s ease'; | |
| } | |
| video.play(); | |
| }; | |
| video.addEventListener('canplay', handleCanPlay); | |
| return () => video.removeEventListener('canplay', handleCanPlay); |
I replaced the call to an external file for the local translated versions. The files are correct in the html:
on live:
on the branch:
they don't appear yet though, but I also have trouble displaying the subtitles in the current main branch..
Also, the "label" argument is supposed to be the language name, I did not find in the code that we have a way to get that from the langcode, do we have something like that already?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation